Skip to content

Fix export import conflicts for llvm #147600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix export import conflicts for llvm #147600

wants to merge 1 commit into from
+3 −1

Conversation

RobertBColton
Copy link

@RobertBColton RobertBColton commented Oct 12, 2025 •
edited by rustbot
Loading

Trying to fix #113050 for #110489 so I can make Windows easier to set up.

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 12, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zalathar
Copy link
Contributor

Zalathar commented Oct 12, 2025 •
edited
Loading

When you're satisfied that PR CI checks have succeeded, please squash any “fixup” commits into the main commit/commits, to make review easier and keep history simple.

@RobertBColton RobertBColton force-pushed the fix-export-import-conflicts branch from 239f780 to b678202 Compare October 12, 2025 08:48
@RobertBColton
Copy link
Author

Done

Zalathar
Zalathar reviewed Oct 12, 2025
View reviewed changes
@nnethercote
Copy link
Contributor

  • Once again, can you squash the two commits together? It's not helpful to have this change as two separate commits, one making a bunch of indentation changes, and the other undoing almost of those changes.
  • Can you briefly explain how this change fixes the problem? Neither the PR description nor the first commit message explains much.
  • This is an area of the compiler I know little about, so I will pass it on to who I think is a more suitable reviewer: r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned nnethercote Oct 13, 2025
bjorn3
bjorn3 requested changes Oct 13, 2025
View reviewed changes
compiler/rustc_codegen_llvm/src/callee.rs

let llfn = if let Some(llfn) = cx.get_declared_value(sym) {
let llfn = if let Some(llfn) = cx.get_declared_value(sym)
&& (!cx.tcx.is_codegened_item(instance.def_id()) || !instance.def_id().is_local())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break things badly. With this change every time you refer to a different raw-dylib import of the same function, LLVM will use a different name for the function in the object file. For example open, open.1, open.2, ... Only the first reference will actually be resolved by the import library. Also in the case of #113050 where someone defines a local function with the same name, if this function is declared first, then the raw-dylib import get the mangled name and if it is declared second, then if another codegen unit tries to call this function it will call the raw-dylib instead.

The only fix for #113050 I can think of is to have rustc mangle raw-dylib imports and also record this mangled name in the import library rustc generates. The issue with that however is that when a user does want to override a raw-dylib import, they can't do that anymore either.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2025
@bjorn3
Copy link
Member

bjorn3 commented Oct 13, 2025

@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@RobertBColton RobertBColton force-pushed the fix-export-import-conflicts branch from 1512b63 to 199cb74 Compare October 13, 2025 19:15
@RobertBColton
Copy link
Author

Squashed again. I'll research this and reply when I have a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zalathar Zalathar Zalathar left review comments

@bjorn3 bjorn3 bjorn3 requested changes

Assignees

@bjorn3 bjorn3

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

#[link(kind="raw-dylib")] mis-compiles if cdylib export has the same name

6 participants

@RobertBColton @rustbot @rust-log-analyzer @Zalathar @nnethercote @bjorn3